Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cpu/native: use async read for stdio_read() #19002

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

cpu/native: use async read for stdio_read() #19002

wants to merge 10 commits into from
+80 −33

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Dec 1, 2022 •
edited
Loading

Contribution description

The real_read() function will block the thread but won't preempt it.
That means all other threads on the same (or higher) priority level are blocked as RIOT still considers the thread that called stdio_read() as running.

Use async_read/isrpipe to properly block the thread when reading from stdin.

Testing procedure

Run a 2nd thread with the same priority as the main thread:

--- a/tests/shell/main.c
+++ b/tests/shell/main.c
@@ -132,10 +132,28 @@ static int _xfa_test2(int argc, char **argv)
 SHELL_COMMAND(xfa_test2, "xfa test command 2", _xfa_test2);
 SHELL_COMMAND(xfa_test1, "xfa test command 1", _xfa_test1);
 
+#include "thread.h"
+#include "ztimer.h"
+
+static char _stack[1024];
+
+static void *_func(void *arg)
+{
+    while (1) {
+        ztimer_sleep(ZTIMER_MSEC, 1000);
+        puts(arg);
+    }
+
+    return NULL;
+}
+
 int main(void)
 {
     printf("test_shell.\n");
 
+    thread_create(_stack, sizeof(_stack), THREAD_PRIORITY_MAIN, THREAD_CREATE_STACKTEST,
+                  _func, "Hello shell", "periodic");
+
     /* define own shell commands */
     shell_run_once(shell_commands, line_buf, sizeof(line_buf));

On master the periodic "Hello shell" message is never printed.

Issues/PRs references

fixes #16834

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform labels Dec 1, 2022
@benpicco benpicco requested review from miri64 and kaspar030 December 1, 2022 16:35
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Dec 1, 2022
@benpicco benpicco requested a review from kfessel December 1, 2022 17:04
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 2, 2022
@riot-ci
Copy link

riot-ci commented Dec 2, 2022 •
edited
Loading

Murdock results

❌ FAILED

41ce809 sys/net/telnet: align API with stdio API

Build failures (10)
Application Target Toolchain Runtime (s) Worker
tests/pkg/libschc native64 llvm 290.08 comsys-minion-0
tests/pkg/libschc native llvm 290.10 mobi7
tests/pkg/libschc native gnu 290.01 mobi7
tests/pkg/libschc native64 gnu 290.11 sarajevo
tests/sys/congure_quic native64 gnu 290.01 breeze
tests/sys/congure_quic native llvm 290.01 mobi3
tests/sys/congure_quic native64 llvm 290.10 mobi6
tests/sys/congure_quic native gnu 290.08 comsys-minion-0
tests/sys/congure_test native gnu 290.10 mobi6
tests/sys/congure_test native64 llvm 290.10 mobi6
Test failures (21)
Application Target Toolchain Runtime (s) Worker
tests/periph/flashpage native64 gnu 0.00 mobi6
tests/periph/gpio native64 gnu 0.00 mobi6
tests/periph/gpio native llvm 0.00 mobi6
tests/pkg/flatbuffers native gnu 0.00 mobi6
tests/pkg/uzlib native64 gnu 0.00 mobi6
tests/pkg/uzlib native64 llvm 0.00 mobi3
tests/sys/atomic_utils native gnu 0.00 mobi6
tests/sys/atomic_utils native64 gnu 0.00 mobi6
tests/sys/atomic_utils native llvm 0.00 mobi6
tests/sys/cpp11_mutex native gnu 0.00 comsys-minion-0
tests/sys/rng native gnu 0.00 breeze
tests/sys/rng native llvm 0.00 comsys-minion-0
tests/sys/rng native64 gnu 0.00 mobi6
tests/sys/senml_cbor native64 llvm 0.00 sarajevo
tests/sys/senml_cbor native64 gnu 0.00 mobi3
tests/sys/senml_cbor native gnu 0.00 mobi6
tests/sys/senml_cbor native llvm 0.00 mobi6
tests/sys/senml_phydat native gnu 0.00 mobi6
tests/sys/senml_phydat native llvm 0.00 breeze
tests/sys/struct_tm_utility native64 gnu 0.00 mobi6

and 57 more test failures...

Artifacts

@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework labels Dec 2, 2022
@benpicco benpicco mentioned this pull request Dec 2, 2022
Merged
@benpicco benpicco requested a review from maribu December 2, 2022 15:22
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from 04cbcef to 9046e8a Compare December 2, 2022 15:31
@benpicco
Copy link
Contributor Author

benpicco commented Dec 2, 2022

@leandrolanzieri do you have an idea how to solve the Kconfig issue?

@benpicco benpicco force-pushed the cpu/native-read_preempt branch from 9046e8a to bfdf538 Compare December 2, 2022 16:26
@github-actions github-actions bot added Area: sys Area: System and removed Area: Kconfig Area: Kconfig integration labels Dec 2, 2022
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from bfdf538 to 42e8122 Compare December 2, 2022 16:34
@github-actions github-actions bot added the Area: examples Area: Example Applications label Dec 2, 2022
gschorcht
gschorcht approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works as expected.

@gschorcht
Copy link
Contributor

Please squash.

@benpicco benpicco force-pushed the cpu/native-read_preempt branch from ebac29d to ad755ef Compare December 20, 2022 19:07
@benpicco
Copy link
Contributor Author

CI will still fail as some tests that use float will now preempt, which triggers #495

@benpicco benpicco mentioned this pull request Jan 5, 2023
Open
@benpicco benpicco mentioned this pull request Jun 8, 2023
Merged
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from fac3e92 to 19166ac Compare June 12, 2023 14:13
@github-actions github-actions bot added the Area: pkg Area: External package ports label Jun 12, 2023
@benpicco benpicco mentioned this pull request Jun 16, 2023
Merged
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from 39dfc42 to b6db3f0 Compare February 8, 2024 20:55
@github-actions github-actions bot added Area: network Area: Networking Area: build system Area: Build system Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: USB Area: Universal Serial Bus labels Feb 8, 2024
benpicco and others added 7 commits February 9, 2024 19:07
@benpicco
cpu/native: use async read for stdio_read()
f13efe5 
The real_read() function will block the thread but won't preempt it.
That means all other thereads on the same (or higher) priority level
are blocked as RIOT still consideres the thread that called stdio_read()
as running.

Use async_read/isrpipe to properly block the thread when reading from
stdin.
@benpicco
cpu/native: bump ASYNC_READ_NUMOF
bef1d37
@benpicco
cpu/native: async_read: make sure not to close stdin on reboot
6927ec2
@benpicco
tests/shell: drop test that only ever worked on native
d932184 
It's no longer working on native, but native behaves more like a
real board now.
@benpicco
tests/pkg_libfixmath_unittests: disable interactive sync on native
a9b8edf
@benpicco
tests/pkg_wolfssl: disable interactive sync on native
3fbf29c
@benpicco
tests/pkg/ubasic: don't use floating point math
b61ec2d
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from b6db3f0 to 17f8f9c Compare February 9, 2024 18:13
@github-actions github-actions bot removed Area: network Area: Networking Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: USB Area: Universal Serial Bus labels Feb 9, 2024
@benpicco benpicco force-pushed the cpu/native-read_preempt branch from 17f8f9c to c65dd56 Compare February 9, 2024 18:14
@github-actions github-actions bot added the Area: network Area: Networking label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@gschorcht gschorcht gschorcht approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@kfessel kfessel Awaiting requested review from kfessel

@jia200x jia200x Awaiting requested review from jia200x

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@maribu maribu Awaiting requested review from maribu

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@bergzand bergzand Awaiting requested review from bergzand

@dylad dylad Awaiting requested review from dylad

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

native getchar is blocking RIOT
4 participants
@benpicco @riot-ci @gschorcht @leandrolanzieri